Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Revert exposing convNormActivation in ops #5472

Merged
merged 2 commits into from
Feb 24, 2022
Merged

Revert exposing convNormActivation in ops #5472

merged 2 commits into from
Feb 24, 2022

Conversation

jdsgomes
Copy link
Contributor

@jdsgomes jdsgomes commented Feb 24, 2022 •
edited
Loading

And remove references in documentation

This change will be cherrypicked for the v12.0 release

@facebook-github-bot
Copy link

facebook-github-bot commented Feb 24, 2022 •
edited
Loading

💊 CI failures summary and remediations

As of commit 3a7695c (more details on the Dr. CI page):

  • 2/2 failures introduced in this PR

2 failures not recognized by patterns:

Job Step Action
CircleCI binary_libtorchvision_ops_android Build 🔁 rerun
CircleCI cmake_macos_cpu curl -o conda.sh https://repo.anaconda.com/miniconda/Miniconda3-latest-MacOSX-x86_64.sh
sh conda.sh -b
source $HOME/miniconda3/bin/activate
conda install -yq conda-build cmake
packaging/build_cmake.sh
🔁 rerun
This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

jdsgomes
jdsgomes commented Feb 24, 2022
View reviewed changes
torchvision/models/optical_flow/raft.py
@@ -6,7 +6,7 @@
from torch import Tensor
from torch.nn.modules.batchnorm import BatchNorm2d
from torch.nn.modules.instancenorm import InstanceNorm2d
from torchvision.ops import ConvNormActivation
from torchvision.ops.misc import ConvNormActivation
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NicolasHug I added you are reviewer as this change touches the RAFT model and is meant to be cherrypicked in the v12.0 release, so I want to double check with you if you spot any problems with this?

For context on this discussion you can have a look at #5445

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be fine and we will literally undo this change immediately after introducing the Conv2dNormActivation

datumbox
datumbox approved these changes Feb 24, 2022
View reviewed changes
Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

torchvision/models/optical_flow/raft.py
@@ -6,7 +6,7 @@
from torch import Tensor
from torch.nn.modules.batchnorm import BatchNorm2d
from torch.nn.modules.instancenorm import InstanceNorm2d
from torchvision.ops import ConvNormActivation
from torchvision.ops.misc import ConvNormActivation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be fine and we will literally undo this change immediately after introducing the Conv2dNormActivation

@jdsgomes jdsgomes merged commit 55a8300 into pytorch:main Feb 24, 2022
@github-actions
Copy link

Hey @jdsgomes!

You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py

jdsgomes added a commit to jdsgomes/vision that referenced this pull request Feb 24, 2022
@jdsgomes
Revert exposing convNormActivation in ops (pytorch#5472)
f9b6050
@jdsgomes jdsgomes mentioned this pull request Feb 24, 2022
Closed
datumbox pushed a commit that referenced this pull request Feb 24, 2022
@jdsgomes
Revert exposing convNormActivation in ops (#5472) (#5474)
9648e07
@jdsgomes jdsgomes removed the request for review from NicolasHug February 24, 2022 20:45
@jdsgomes jdsgomes added module: ops revert(ed) For reverted PRs, and PRs that revert other PRs labels Feb 24, 2022
facebook-github-bot pushed a commit that referenced this pull request Feb 25, 2022
@prabhat00155 @facebook-github-bot
[fbsync] Revert exposing convNormActivation in ops (#5472)
b846f97 
Reviewed By: jdsgomes

Differential Revision: D34475311

fbshipit-source-id: 13ea879714fd265166e42409956a73918ac0bd91
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@datumbox datumbox datumbox approved these changes

Assignees
No one assigned
Labels
ciflow/default cla signed module: ops revert(ed) For reverted PRs, and PRs that revert other PRs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jdsgomes @facebook-github-bot @datumbox